Skip to content

[1.17] Add Entity Visibility API#6000

Closed
Janmm14 wants to merge 4 commits into
PaperMC:masterfrom
Janmm14:feature-entity-visibility-api
Closed

[1.17] Add Entity Visibility API#6000
Janmm14 wants to merge 4 commits into
PaperMC:masterfrom
Janmm14:feature-entity-visibility-api

Conversation

@Janmm14
Copy link
Copy Markdown

@Janmm14 Janmm14 commented Jun 25, 2021

Closes #5973

This is a port to 1.17 of #5993

@Janmm14 Janmm14 requested review from a team as code owners June 25, 2021 21:35
Copy link
Copy Markdown
Contributor

@Trigary Trigary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately Level#sendParticles also exists and is used by entities (eg. Squid#spawnInk, LivingEntity#checkFallDamage). You would need to handle that similarly to sounds.

@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Jun 26, 2021

Unfortunately Level#sendParticles also exists and is used by entities (eg. Squid#spawnInk, LivingEntity#checkFallDamage). You would need to handle that similarly to sounds.

I've now edited sendParticle and playSound for all entities

@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Jun 29, 2021

stuff fixed

@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Jul 12, 2021

bump

@stale
Copy link
Copy Markdown

stale Bot commented Sep 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Sep 10, 2021

.

@stale stale Bot removed the resolution: stale label Sep 10, 2021
@Chew Chew added the type: feature Request for a new Feature. label Oct 15, 2021
Comment thread patches/api/0321-Add-Entity-Visibility-API.patch Outdated
com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> lastTrackerCandidates;

- final void updatePlayers(com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> newTrackerCandidates) {
+ public final void updatePlayers(com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> newTrackerCandidates) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing a paper comment, and really, it should be a AT

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"AT"?

Copy link
Copy Markdown
Contributor

@lynxplay lynxplay Oct 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot use AT for this one because method is added by patch 0431

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then go back to the patch that adds it and change the visibility there.

Comment thread patches/server/0719-Add-Entity-Visibility-API.patch Outdated
public boolean isTicking();
// Paper end
+
+ // Paper start - Entity Visibility API
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a new comment "block" here, move it into the already existing one

Set<Player> getTrackedPlayers();
// Paper end

+ // Paper start - Entity Visibility API
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Copy Markdown
Author

@Janmm14 Janmm14 Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But litterally a couple lines above the same thing happens already and I think that it also helps to keep patches a little more individual.

}
// Paper end
+
+ // Paper start - Entity Visibility API
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment thread patches/server/0719-Add-Entity-Visibility-API.patch
@jaylawl
Copy link
Copy Markdown

jaylawl commented Nov 10, 2021

Please don't let this die

@Janmm14 Janmm14 requested review from Machine-Maker, MiniDigger and lynxplay and removed request for a team November 29, 2021 21:38
@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Nov 29, 2021

soo I remembered again I had this thing open, please check again

Copy link
Copy Markdown
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a note, upstream added something that might conflict with this. Probably needs to be considered somehow here.

com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> lastTrackerCandidates;

- final void updatePlayers(com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> newTrackerCandidates) {
+ public final void updatePlayers(com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet<ServerPlayer> newTrackerCandidates) { // Paper - Entity Visibility API - packageprivate -> public
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if you saw my comment here, but if this is a method added by paper in a previous patch, you should go back to that patch and change the visibility there to reduce the diff.

@Janmm14
Copy link
Copy Markdown
Author

Janmm14 commented Nov 29, 2021

@Machine-Maker wrote:

Just as a note, upstream added something that might conflict with this. Probably needs to be considered somehow here.

Looks like this patch got a little redundant.

I will create a PR to upstream with the details upstream ignored (sounds, particles).
As they accepted the base idea, the fixes to it will hopefully be accepted as well.

Edit: So far I did not create any upstream PR for fixes like sound and I don't plan to in the near future, so if anyone wants to do it instead, go ahead. If not, also fine.

@Janmm14 Janmm14 mentioned this pull request Mar 13, 2022
2 tasks
@Owen1212055
Copy link
Copy Markdown
Member

Seeing as upstream has merged most of what this pr was meant to bring this will be closed.

The whole "hide by default behavior" seems to be covered by another issue which has some PRs for it as well. #7395

Because blocking sounds/particles might still be a wanted feature, feel free to open a separate PR (here or upstream) that covers that specific area while using the current visibility API that upstream has.

Thank you for your contribution, and sorry we couldn't get to merge this. ☹️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Entity Visibility API

9 participants